Skip to content

Conversation

@tonykode
Copy link

@tonykode tonykode commented Apr 1, 2022

No description provided.

Shatnerz and others added 30 commits February 9, 2022 22:04
Improve the docs by doing:
- Use markdown heading for `Errors` section
- Use 100 character lines
The test script is incorrectly named in our contributor docs. Fix it up
with the correct name.
Some code has only two spaces of indentation, we favour 4 in bash
scripts.
We currently build the docs as a separate CI job, we can however just do
it as part of the `Tests` job using the nightly toolchain.

Conditionally build the docs based on a `DO_DOCS` env var.

Note, uses `--cfg docsrs` so can only be built run with nightly toolchain.
The policy module contains a single `mod.rs` file, this is unnecessary,
we can simply use `policy.rs` and flatten the module.
Put the calls to `impl_hashencode` in the same order, and with the same
whitespace, as the calls to `hash_newtype`. This makes groking the file
easier because its quick to glance down the types and see which ones
implement hashencode (consensus_encode/decode) and which ones do not.
Improve docs on `ClassifyContext` by doing:

- Separate brief doc line from the rest
- Use uniform backticks on opcodes
Whitespace only, no code changes.
Add full stops to all lines of rustdocs in the `blockdata::opcodes`
module.
7638d59 Improve rusntdocs for *_hash_ty methods (Tobin Harding)

Pull request description:

  Recently we added two new methods with rustdocs. The rustdocs have a few minor things that can be improved.

  - Use 'Returns' section for both success and error path returns
  - Use markdown heading for section
  - [Subjectively] improve wording of docs

  @rish-singhal this PR is minor rustdocs stuff that I have recently been learning myself. I hope this makes your future patches easier to do in regards to the docs. FTR there are `# Returns` sections and `# Examples` sections that one can use, no set rules as far as I can tell. I chose `# Returns` in this case because it seemed to work. Please feel free to NACK the wording changes if you do not agree.

ACKs for top commit:
  apoelstra:
    ACK 7638d59
  Kixunil:
    ACK 7638d59

Tree-SHA512: ac1629647a39789e9a162b54440798507c74501ef0a5014bc6dfe2cf5779588a304c059433b8387bbe06f413f0712d1204e4b7d912c79e31f9485f6ddcf9ceba
Currently function contains nested `if` clauses that arguably obfuscate
the code. We can make the code easier to read by pulling out the error
paths and returning them higher up in the function.

Refactor only, no logic changes.
The returns part of the function docs appears to be stale, remove it.
Improve wording of rustdocs while we are at it.
Improve the rustdocs for the `blockdata::block` module:

- Use full sentences (capitalisation and full stop)
- Use third person tense instead of imperative
- Improve wording if needed
Code comment contains an off-by-one error, update it to the correct
value '61'.
The two methods `encode_signing_data_to` and `signature_hash` use the
same docs (one is a public helper for the other). The docs have gotten a
bit stale (refer to deprecated types).

Instead of duplicating all the text, add a statement pointing readers
from the docs of `signature_hash` to the docs on
`encode_signing_data_to`.
`assert!` already checks a boolean, it is redundant to use `assert_eq!`
and pass in `true`.

Remove redundant usage of `assert_eq!(foo, true)`.
The line of code `let mut have_witness = self.input.is_empty();` is
puzzling if one does not know _why_ we serialize in BIP141 style when
there are no inputs.

Add a code comment to save devs spending time trying to work out _why_
this is correct.
Improve the rustdocs for the `blockdata::transaction` module:

- Use full sentences (capitalisation and full stop)
- Use third person tense instead of imperative
- Improve wording/grammar
- Use backticks in links
- Use 100 character column width if it improves readability

Nothing too controversial here :)
Improve the docs in the `blockdata::script` module by doing:

- Use full sentences (use capitals and full stops)
- Improve grammar/wording if necessary
- Remove incorrect/unneeded comments
- Fix layout of rustdoc i.e., use brief and description sections
- Use 100 line character width if it makes the comment look better
- Use third person instead of imperative tense
This module has been deprecated in commit 1ffdce9 in August 2020, it is
safe to delete it now.

Fixes: #322
aaf587d Use correct opcode count (Tobin Harding)

Pull request description:

  Code comment contains an off-by-one error, update it to the correct value '61'.

  Fixes: #866

ACKs for top commit:
  apoelstra:
    utACK aaf587d
  Kixunil:
    ACK aaf587d

Tree-SHA512: 0306f1bbad904c1bfb26ce69758114dd94ee748c8733094fe94b1e1072be84a823a906ecc2046c30aa23c04e762199418bfeab3b63f3dc0c25e2c582813edbb4
Update our `rust-secp256k1` dependency to the latest version.

Requires doing:

- Add a new variant to `Error` for the case where parity of the internal
  key is an invalid value (not 0 or 1).
- Use non-deprecated const
d68531d Update secp256k1 dependency (Tobin Harding)

Pull request description:

  Update our `rust-secp256k1` dependency to the latest released version.

  Requires doing:

  - Add a new variant to `Error` for the case where parity of the internal key is an invalid value (not 0 or 1).
  - Use non-deprecated const

  Please check the error change carefully, this error does relate _only_ to the parity of an internal key, right?

ACKs for top commit:
  apoelstra:
    ACK d68531d
  dr-orlovsky:
    ACK d68531d

Tree-SHA512: 2552b07c0ccc065ced412caadaa0e9d8d77b5f2ce3698b7f53367a9f183557172526c154594c1c706e229da1bab67d11d88255cfd1fe3aac3e16888fe2948aae
4dcbef6 Improve docs: script module (Tobin Harding)

Pull request description:

  Improve the docs in the `blockdata::script` module by doing:

  - Use full sentences (use capitals and full stops)
  - Improve grammar/wording if necessary
  - Remove incorrect/unneeded comments
  - Fix layout of rustdoc i.e., use brief and description sections
  - Use 100 line character width if it makes the comment look better
  - Use third person instead of imperative tense

  ## Note to reviewers

  Sorry to be a bore and request review on all these docs fixes, this one is all in a single patch which makes it a bit harder to review. It is very similar in content to all the others that are open right now so I'm going to be a bit rude and leave it like this. Please say if this is even slightly putting too much demand on you review time.

ACKs for top commit:
  apoelstra:
    ACK 4dcbef6
  dr-orlovsky:
    ACK 4dcbef6

Tree-SHA512: 49fa1d88c4b97decbc563747ba166fe95698da6a634801ccf5f99fd67a4a907067dbf0a4d64e7773d5d5b04aef404167b6cc911382363247d15a61cef5d8965c
e503f14 Improve docs: blockdata::transaction (Tobin Harding)
f02b3a8 Add code comment for emtpy input (Tobin Harding)
6a0ec1a Remove redundant _eq (Tobin Harding)
3bcc146 Improve docs: encode_signing_data_to/signature_hash (Tobin Harding)

Pull request description:

  Do some cleanups to the docs in `blockdata::transaction`. Patch 1 needs the most careful review please. The rest should not be too controversial.

ACKs for top commit:
  apoelstra:
    ACK e503f14
  dr-orlovsky:
    ACK e503f14

Tree-SHA512: 3953226e1b7f0db0371b1902888407a48531688bf8ed08539a0090f369b491b130d70b2fae859878ef178a397cefe0ee2a15f3358afc990a2776194cc2b3882b
146d5e8 Improve docs for blockdata::block (Tobin Harding)
f03092c Fix erroneous function rustdoc (Tobin Harding)
5464848 Refactor check_witness_commitment (Tobin Harding)

Pull request description:

  Do some clean ups to the `blockdata::block` module.

  - Patch 1: Change predicate names (API breaking, could be seen as unnecessarily changing the API), can remove if NACK'd
  - Patch 2: Refactor to assist code clarity
  - Patch 3 and 4: are docs improvements, shouldn't be too controversial

ACKs for top commit:
  apoelstra:
    ACK 146d5e8
  dr-orlovsky:
    ACK 146d5e8

Tree-SHA512: 65cc414857c4569a389638b53eb99ed629bf67ae1d8ebdc9023e5974bb26902d4de41ec311bef3b5c895229d7d0df78d469a84c1e94fc0b7be7435338f0d510a
0d36455 Build the docs with test.sh (Tobin Harding)
8163497 Use correct indentation (Tobin Harding)
3786680 Use correct script name (Tobin Harding)

Pull request description:

  We currently build the docs as a separate CI job, we can however just do it as part of the `Tests` job using the nightly toolchain.

  Conditionally build the docs based on a `DO_DOCS` env var.

  Note, uses `--cfg docsrs` so can only be built run with nightly toolchain.

  - Patch 1: Fixes the incorrect file naming `ci.sh` -> `test.sh` in `CONTRIBUTING.md`.
  - Patch 2 - 4: Do trivial cleanup of `test.sh`.
  - Patch 5: Does the fix described above.

  Resolves: #850

ACKs for top commit:
  Kixunil:
    ACK 0d36455
  apoelstra:
    ACK 0d36455
  dr-orlovsky:
    ACK 0d36455

Tree-SHA512: c33c8df687c2115477eae9888b80d4e744d7b68b598694cf17220dd11098f33ba23c0b33e6f7d291572187942c472d1bc9cbb5217d3d83d41906a97c0b3417e5
f4886af Add full stops to docs (Tobin Harding)
f01f047 Remove unnecessary newlines (Tobin Harding)
8a1cc2c Improve docs on ClassifyContext (Tobin Harding)

Pull request description:

  Do some clean ups to the `blockdata::opcodes` module. Patch 3 is big but it should be quick to review because I made all the boring 'add full stops' changes in a single commit.

ACKs for top commit:
  Kixunil:
    ACK f4886af
  apoelstra:
    ACK f4886af
  dr-orlovsky:
    ACK f4886af

Tree-SHA512: b30f36bd06a028b6bbc24a64849c0788a9223760907bdcb3765af1742a228f630cc7666ed66fa2afd8fb6c96e3cf416e9bd9d2a3b6c72c6e47a16399a856fca1
tcharding and others added 28 commits March 28, 2022 10:43
The functions `from_u32_standard` and `from_u32_consensus` smell a bit
like hungarian notation. We can look at the method definition to see
that the methods accept `u32` arguments without mentioning that in the
method names.

Remove `_u32_` from the method names. This brings the `from_*` methods
in line  with the `to_standard` method also.
Improve the `PsbtSigHashType` conversion methods by doing:

- Re-name `inner` -> `to_u32` as per Rust convention
- Add `from_u32` method

Note, we explicitly do _not_ use suffix 'consensus' because these
conversion methods make no guarantees about the validity of the
underlying `u32`.
It is possible, although not immediately obvious, that it is possible to
create a `PsbtSigHashType` with a non-standard value.

Add a unit test to show this and also catch any regressions if we
accidental change this logic.
This allows users to create TaprootSpendInfo using NodeInfo. This
offers an alternative to TaprootBuilder.
8e2422f Add unit test for deserialize non-standard sighash (Tobin Harding)
e05776f Improve PsbtSigHashType conversion methods (Tobin Harding)
ac46289 Remove hungarian-ish notation (Tobin Harding)
5646826 Remove deprecated conversion method (Tobin Harding)
d1753d7 Rename as_u32 -> to_u32 (Tobin Harding)
2bd71c3 Remove From<EcdsaSigHashType> for u32 (Tobin Harding)

Pull request description:

  This PR has evolved into a full blown clean up of the conversion methods for all three sighash types based on the discussion below.

  Everything is split up into very small patches to aid review (and bikeshedding, this PR is almost totally just naming things).

  EDIT: I'm not convinced this should be an RC-fix, the changes are too wide spread now. It started as just a single method rename. Leaving label as is for others to consider.

ACKs for top commit:
  sanket1729:
    utACK 8e2422f.
  dr-orlovsky:
    ACK 8e2422f

Tree-SHA512: 8269bdf6d00a98b472a720b891f25728d5dedf3b2648aa60a461efdbdf37d883a1824a15f1838a792329ec9ac50a9c05618ab2a34cf201fcf63e4ad145955571
Post-merge #796 follow-up. Feel free to add other changes/nits which hadn't get into #796.
c3d30d5 Remove deprecated method use for sighash conversion (Dr. Maxim Orlovsky)

Pull request description:

  Post-merge #796 follow-up. Feel free to add other changes/nits which hadn't get into #796.

ACKs for top commit:
  tcharding:
    ACK c3d30d5
  sanket1729:
    ACK c3d30d5
  apoelstra:
    ACK c3d30d5

Tree-SHA512: 7418f90bad9ce43b5504a3e45f06fecd636f62f2f7bd75bfc27e29faa181202595ed9b5175866e0fce01a301ea34c2b07afb16e658757215823965e7e1440c2e
e27f8ff TapTree iterator implementation (Dr Maxim Orlovsky)

Pull request description:

  Implemented after @sanket1729 suggestion in #895 (comment)

  Iterates all scripts present in TapTree in DFS order returning `(depth, script)` pairs.

  I propose to have it as an RC fix since this functionality is really lacking and may be required for many wallets working with Taproot PSBT even outside of the scope where I originally needed it (OP_RETURN tweaks for TapTree described in #895)

ACKs for top commit:
  sanket1729:
    utACK e27f8ff.
  apoelstra:
    ACK e27f8ff

Tree-SHA512: b398e468a10534561297f22dba47e340391069734a41999edd85d726890752035053690a22014402879ea40b948160f00310f78771443d382c0bbaf0201dfbe5
208eb65 Make NodeInfo API public (sanket1729)

Pull request description:

  Reported by @shesek. Users might find it convenient to manually construct the tree using `NodeInfo` API

  ```rust
  let leaf1 = NodeInfo::from_leaf_with_ver();
  let leaf2 = NodeInfo::from_leaf_with_ver();

  let root = NodeInfo::combine(leaf1, leaf2);
  let spend_info = TaprootSpendInfo::from_node_info(&secp, internal_key, root);
  ```

ACKs for top commit:
  dr-orlovsky:
    ACK 208eb65
  apoelstra:
    ACK 208eb65

Tree-SHA512: b5a6b26e0d4a637f7ad6e987976b31b00d3567feca85f1a0bf63aa03603aded0ddae6578b1cabc1056870a596b8cb1a83e4ef3f45802e03da80c3d58d9bab1f1
ec17ec3 Move with_huffman_tree logic to TaprootBuilder (Jeremy Rubin)

Pull request description:

  .

ACKs for top commit:
  apoelstra:
    ACK ec17ec3
  dr-orlovsky:
    utACK ec17ec3

Tree-SHA512: 67a013124267f64bfae0b2007418ad59a42ae64d8b95e23c1d86cc7d96b0dd3b48deb255ce7bb839ef9a4d4f2e3a42d691d2d2430eb7791e01f992635773cc96
8dabe3e Taproot Huffman tree builder u64->u32 fixes (Dr Maxim Orlovsky)

Pull request description:

  Follow-up fixes for #909

ACKs for top commit:
  sanket1729:
    ACK 8dabe3e
  apoelstra:
    ACK 8dabe3e

Tree-SHA512: 0e05b2183c7746ed57b0f585abf769d10b638840e9b8b0de07e718f451f349c15c06e223930fd51b192f1328734fabf242f1fa701518ebc57ceed4b4d85c8dbe
51fef76 feat: Add Address.is_related_to_pubkey() (Andrew Ahlers)

Pull request description:

  ## Motivation

  This is addressing the second half of this comment: #684 (comment)

  > but would accept a PR (or two PRs) that returns Result<bool, UnsupportedAddress> and a method to check if a PublicKey is associated with an address.

  (The first half was addressed [here](#819))

  These changes will help build out and improve message signature verification. We don't necessarily need to add it to this crate but it allows for easy verification with something such as:
  1. recovering a pubkey
  2. checking if that pubkey relates to the given address

  ## Possible Improvements

  - There is likely a better name than `is_related_to_secp256k1_key()`
  - This could drop the `secp256k1` part of the name and take in a Pubkey enum that also supports Schnorr pubkeys and then this could be used for taproot addresses as well. This felt like a much larger change that will likely get turned down. Verifying taproot is simple enough and if absolutely desired, similar functions can be added for schnorr keys (tweaked and untweaked)

ACKs for top commit:
  Kixunil:
    ACK 51fef76 for merging after TR
  apoelstra:
    ACK 51fef76

Tree-SHA512: c9ab8c0f101fb4c647713e7f500656617025d8741676e8eb8a3132009dde9937d50cf9ac3d8055feb14452324a292397e46639cbaca71cac77af4b06dc42d09d
…gHashTypes

992857a PsbtSighashType unit tests (Dr Maxim Orlovsky)
5be1cdb PsbtSigHashType Display and FromStr implementation (Dr Maxim Orlovsky)
7cdcdaa Support SIGHASH_RESERVED in SchnorrSigHashType::from_u8 (Dr Maxim Orlovsky)

Pull request description:

  The newly introduced `PsbtSigHashType` uses very different serde formatting from previously used `EcdsaSigHashType`; for instance it does not output human-readable sighash. This is especially obvious when printing out PSBT as JSON/YAML object and is a breaking change from the `0.27`. Serde human-readable implementation requires `Display/FromStr`, which were also absent.

ACKs for top commit:
  sanket1729:
    ACK 992857a. This is much better
  apoelstra:
    ACK 992857a

Tree-SHA512: 71a46471f34b5481e4c1273a66846f59d61bfd98fcb65e7823ca216ff0dd419d81ca86d99c7aaf674fcfe2b1c010e899c8e74328f60a1e809015c663c453cc89
174a99c Implement serde for TweakedKeyPair (Dr Maxim Orlovsky)
df3297c Implement derives for TweakedKeyPair (Dr Maxim Orlovsky)

Pull request description:

  We forgot about them and marked as TODO. This is clearly an RC fix

ACKs for top commit:
  apoelstra:
    ACK 174a99c
  sanket1729:
    ACK 174a99c

Tree-SHA512: 6cd446f1b73a9f381db976dcf77d75a108e60f5a521a0d387052779be5ac98ba6b82fb3a39dc58e5529ffcc4fb2fef2a037443dc1afde8309716096f97408a78
0d29c83 rust-bitcoin 0.28.0-rc.2 (sanket1729)
b3e612a Remove misc whitespace (sanket1729)

Pull request description:

  This does not include a changelog because rc.1 did not have changelog.

ACKs for top commit:
  dr-orlovsky:
    ACK 0d29c83
  apoelstra:
    ACK 0d29c83

Tree-SHA512: ec0858c0b075c023abb6a10e377b07b5fad56a683efb68450afe1270a74a705349c0050526125d8dd6bed08fc9aed263f4dde68f3c8ade6c76e6143da5f12691
As has been done in other places in the codebase; improve the docs in
the `taproot` module by doing:

- Use full sentences (capital letters + full stops)
- Use back ticks and links for types where appropriate
- Fix grammar
- Fix stale docs
- Use third person for describing functions
- Use 100 character line width
- Use markdown sections (`# Examples`, `# Returns`) where appropriate
- Separate brief heading from extended description when appropriate
- Use `///` for all functions/types (both private and public)

I also did:

- Build the docs and check all the links
- Read all the built docs, check for sanity and pretty-ness
We have some text quoted directly from BIP341, this text is on the net
if folk wish to read it, we don't need it in the source code.
We deprecated the `bip143::SigHashCache` in

```
commit 53d0e17
Author: <elided>
Date:   Fri Jul 16 10:44:18 2021 +0200

    Deprecate bip143::SigHashCache in favor of sighash::SigHashCache

    ...
```

This means these changes are unreleased so the deprecated since version
should be the upcoming 0.28 release.
8d602b8 Fix deprecated since version (Tobin Harding)

Pull request description:

  We deprecated the `bip143::SigHashCache` in

  ```
  commit 53d0e17
  Author: <elided>
  Date:   Fri Jul 16 10:44:18 2021 +0200

      Deprecate bip143::SigHashCache in favor of sighash::SigHashCache

      ...
  ```

  This means these changes are unreleased so the deprecated since version
  should be the upcoming 0.28 release.

ACKs for top commit:
  sanket1729:
    Nice catch! ACK 8d602b8
  dr-orlovsky:
    ACK 8d602b8

Tree-SHA512: 7fba5b542de0d03e519a77204908cacb78c160bc41e02010b2bb258d8620d988dbc5d686a7b9f5144bf5afea414cb2c1b0c0f4e817c08b16f4c48c6f8d0de427
c25eddd Remove unnecessary documentation (Tobin Harding)
8631474 Improve docs in taproot module (Tobin Harding)

Pull request description:

  I should have done this PR a month ago, my bad. This one is kind of important IMO because we are going to have so many people looking at this part of the code soon as we release.

  As has been done in other places in the codebase; improve the docs in the `taproot` module by doing:

  - Use full sentences (capital letters + full stops)
  - Use back ticks and links for types where appropriate
  - Fix grammar
  - Fix stale docs
  - Use third person for describing functions
  - Use 100 character line width
  - Use markdown sections (`# Examples`, `# Returns`) where appropriate
  - Separate brief heading from extended description when appropriate
  - Use `///` for all functions/types (both private and public)

  I also did:

  - Build the docs and check all the links
  - Read all the built docs, check for sanity and pretty-ness

  Its all in one patch, I couldn't really tease it apart. I can try a bit harder if it proves too annoying to review.

ACKs for top commit:
  sanket1729:
    ACK c25eddd
  dr-orlovsky:
    ACK c25eddd
  apoelstra:
    ACK c25eddd

Tree-SHA512: 72f35bf8779392060388db985df5abc42a89796eaad1eafd08ea50b635d469fbd07a53ff253cdf27ad4d4baed7d37cec6ea1da1aece3672b9447f87181e218f8
@tonykode tonykode merged commit ccf4cb5 into dashpay:master Apr 1, 2022
shumkov pushed a commit that referenced this pull request Feb 28, 2025
chore: replace bitcoin with dashcore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants